Conversation
- Add visible stderr output during binary download (P0) - Add gRPC health probe after socket appears in ensure_running (P0) - Add retry with exponential backoff for download (3 attempts) (P1) - Update test mock for retry-aware download error handling
|
✅ Documentation validation passed!
|
|
✅ All checks passed! Ready for review. |
There was a problem hiding this comment.
Pull request overview
Improves the first-run capiscio-core binary installation and startup experience in the Python SDK by making downloads visible to users, adding retry/backoff on download failures, and strengthening ensure_running() to verify gRPC readiness (not just socket existence).
Changes:
- Emit download/install progress to stderr during first-run binary download.
- Add up to 3 download attempts with exponential backoff on failure.
- Add a gRPC channel readiness probe after the socket appears; update the unit test to be retry-aware.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
capiscio_sdk/_rpc/process.py |
Adds stderr progress output, download retry/backoff, and a gRPC readiness check in ensure_running(). |
tests/unit/test_process.py |
Updates the HTTP error download test setup to accommodate the new retry logic. |
Comments suppressed due to low confidence (1)
tests/unit/test_process.py:210
- This test is now described as covering retry behavior, but it only asserts that a
RuntimeErroris raised. It should also assert retry-specific effects (e.g.,httpx.streamcalled 3 times andtime.sleepcalled with the expected backoff delays) so that the new retry logic can’t regress without failing the test.
@patch("capiscio_sdk._rpc.process.time.sleep")
@patch("httpx.stream")
def test_download_binary_http_error(self, mock_stream, mock_sleep):
"""Test download handles HTTP errors with retries."""
pm = ProcessManager()
with patch("capiscio_sdk._rpc.process.platform.system", return_value="Linux"):
with patch("capiscio_sdk._rpc.process.platform.machine", return_value="x86_64"):
with patch.object(ProcessManager, "_get_cached_binary_path") as mock_cached:
mock_path = MagicMock()
# exists() called: once before loop, then once per attempt for cleanup
mock_path.exists.return_value = False
mock_path.parent = MagicMock()
mock_cached.return_value = mock_path
# Mock HTTP error
mock_response = MagicMock()
mock_response.raise_for_status.side_effect = Exception("404 Not Found")
mock_stream.return_value.__enter__.return_value = mock_response
with pytest.raises(RuntimeError, match="Failed to download capiscio-core"):
pm._download_binary()
You can also share your feedback on Copilot code review. Take the survey.
| try: | ||
| channel = grpc.insecure_channel(addr) | ||
| grpc.channel_ready_future(channel).result(timeout=min(1.0, remaining)) | ||
| channel.close() | ||
| break | ||
| except grpc.FutureTimeoutError: | ||
| pass | ||
| except Exception: | ||
| time.sleep(0.1) |
There was a problem hiding this comment.
In the gRPC readiness loop, the channel is only closed on the success path. If channel_ready_future(...).result(...) raises (including FutureTimeoutError), the channel stays open, which can leak file descriptors/sockets and keep piling up across retries. Consider creating the channel once per iteration and always closing it in a finally block, and add a small sleep/backoff on FutureTimeoutError to avoid a tight spin loop.
| try: | |
| channel = grpc.insecure_channel(addr) | |
| grpc.channel_ready_future(channel).result(timeout=min(1.0, remaining)) | |
| channel.close() | |
| break | |
| except grpc.FutureTimeoutError: | |
| pass | |
| except Exception: | |
| time.sleep(0.1) | |
| channel = grpc.insecure_channel(addr) | |
| try: | |
| iter_remaining = deadline - time.time() | |
| if iter_remaining <= 0: | |
| break | |
| grpc.channel_ready_future(channel).result(timeout=min(1.0, iter_remaining)) | |
| break | |
| except grpc.FutureTimeoutError: | |
| # Backoff slightly to avoid a tight spin loop while waiting for readiness | |
| time.sleep(0.1) | |
| except Exception: | |
| time.sleep(0.1) | |
| finally: | |
| channel.close() |
capiscio_sdk/_rpc/process.py
Outdated
| try: | ||
| channel = grpc.insecure_channel(addr) | ||
| grpc.channel_ready_future(channel).result(timeout=min(1.0, remaining)) |
There was a problem hiding this comment.
remaining is computed once before the readiness loop, but each iteration uses result(timeout=min(1.0, remaining)). Near the end of the overall deadline, this can block for up to 1s and exceed the intended timeout. Recompute time_left = deadline - time.time() inside the loop and use that for the per-iteration result() timeout.
| try: | |
| channel = grpc.insecure_channel(addr) | |
| grpc.channel_ready_future(channel).result(timeout=min(1.0, remaining)) | |
| # Recompute time left for each iteration to avoid overshooting the overall timeout | |
| time_left = deadline - time.time() | |
| if time_left <= 0: | |
| break | |
| try: | |
| channel = grpc.insecure_channel(addr) | |
| grpc.channel_ready_future(channel).result(timeout=min(1.0, time_left)) |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
|
✅ SDK server contract tests passed (test_server_integration.py). Cross-product scenarios are validated in capiscio-e2e-tests. |
- Close channel in finally block to prevent FD leaks on exceptions - Recompute time_left per iteration to avoid overshooting timeout - Add backoff sleep on FutureTimeoutError to prevent tight spin
|
✅ Documentation validation passed!
|
|
✅ All checks passed! Ready for review. |
|
✅ SDK server contract tests passed (test_server_integration.py). Cross-product scenarios are validated in capiscio-e2e-tests. |
Summary
Fixes binary install experience issues identified in the install audit.
P0 Fixes
ensure_running()now verifies the gRPC server is actually accepting connections after the socket file appears (not just checking file existence)P1 Fixes
Files Changed
capiscio_sdk/_rpc/process.py— Download visibility, health probe, retry logictests/unit/test_process.py— Updated mock for retry-aware error testTesting
find_binary_*tests unrelated to this PR)